Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Column values sort #349

Merged
merged 5 commits into from
Mar 31, 2021
Merged

Column values sort #349

merged 5 commits into from
Mar 31, 2021

Conversation

clrcrl
Copy link
Contributor

@clrcrl clrcrl commented Mar 30, 2021 •
edited
Loading

Opening in place of #289 — needed to rebase to get it to work, and while I was here I simplified the implementation a little

This is a:

  • new functionality — please ensure the base branch is the latest dev/ branch
  • a breaking change — please ensure the base branch is the latest dev/ branch — changes positional argumenet order

Description & motivation

Implement an order_by arg to get_column_values. Fix up docs while here

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

@clrcrl clrcrl requested a review from jtcohen6 March 30, 2021 15:45
@clrcrl
Copy link
Contributor Author

clrcrl commented Mar 30, 2021

@jtcohen6 — can I get your eyes on this. Namely, I'm curious whether order_by feels like the right argument name (sort?). We also at some point should deprecate the table argument name but that feels like more than I want to do right now to get this backlog down

@clrcrl clrcrl mentioned this pull request Mar 30, 2021
Closed
11 tasks
@jtcohen6
Copy link
Contributor

jtcohen6 commented Mar 31, 2021 •
edited
Loading

Namely, I'm curious whether order_by feels like the right argument name (sort?).

Thinking only about the nomenclature, I'd feel equally fine about order_by and sort_by. I like order_by because it neatly maps to what's happening being run behind the scenes: it's actually order by in the query, rather than (say) a sort Jinja filter on the returned query results.

@clrcrl clrcrl merged commit c304881 into dev/0.7.0 Mar 31, 2021
@clrcrl clrcrl deleted the column-values-sort branch March 31, 2021 15:56
@clrcrl clrcrl mentioned this pull request May 18, 2021
Merged
clrcrl pushed a commit that referenced this pull request May 19, 2021
@clrcrl
Add order_by argument to get_column_values (#349)
c02c009
clrcrl pushed a commit that referenced this pull request May 19, 2021
@clrcrl
Add order_by argument to get_column_values (#349)
afaa6e1
jtcohen6 added a commit that referenced this pull request Jun 6, 2021
@clrcrl @jtcohen6 @ChristopheDuong
0.7.0 into main (#372)
a729044 
* Tidy up changelog

* Add 0.7.0 entry to changelog

* Add order_by argument to get_column_values (#349)

* Add slugify macro to utils, use in pivot macro (#314)

* 0.20.0 compatibility (#371)

* Explicitly redefine Redshift -> default

* Upgrade generic tests

* Rm namespaces macro. New dispatch syntax

* Run tests with 0.20.0rc1

* Update changelog, readme

Co-authored-by: Jeremy Cohen <[email protected]>

* Simplify concat (#373)

* Postgres also have an alternative concat binary operation (#296)

* Update default implementation of concat macro

Co-authored-by: Christophe Duong <[email protected]>

Co-authored-by: Jeremy Cohen <[email protected]>
Co-authored-by: Christophe Duong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtcohen6 jtcohen6 Awaiting requested review from jtcohen6

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clrcrl @jtcohen6 @clausherther